refactor: sensors abstraction - #4

Don't merge yet, just putting it here for early review

Looks good, assuming you just need to test this? Also, I feel like the "handle_as" functions would be better named "handle_get_as"

I guess I am just not 100% convinced yet that we need this level of abstraction for our sensors? Do we get a lot out of it in terms of maintainability and ease of understanding code? I wonder if it would be easier to just make specific implementations for each sensor, and let it be up to the device to determine how they want to use all that.

Just to be clear, I am by no means the architect of this code and you should talk to Dhruv about anything major with regard to that

The main pro of abstracting them to this thin api class is that we keep all the #ifdefs in one place. The sensors don't (and shouldn't) care about what protocol is used. With previous approaches one of the big issues was how to handle

1. compilation error due to missing protocol (no SPI, or no UART)
2. choosing between protocols (this is especially more difficult with flash)

This abstraction, to some degree, resolves 1. by collecting all #ifdefs and protocol definitions in one file, and 2. by allowing to simply use the api->read and api->write, which are defined by the protocol_api which you can just switch with another such struct to change which protocol is used.

On the point of maintainability and readability, we concretely defined three actions that every sensor/device has to do

1. writes: for flash this is writing data or for a sensor this could be writing a register value
2. reads: for both flash and sensor, this is reading values, either from a register or from the NAND
3. command: this is usually for flash, which is to send a single command without expecting any further writes or reads

This means every implementation after can simply think of writes, reads, and commands rather than the actual HAL commands. Bosch has already done something similar with their own read and write callbacks, albeit it was mandatory on their part.

On maintainability, since this is the way every device/sensor communicates, we can have the implementation in a single place, with much more eyes on it, rather than the same implementation scattered across the codebase. Sorry for the long reply and rambling.

No need to apologize! I agree with the sentiment of wanting things in a core place. I have an idea to consider, I know you all seem to hate cpp but please, just hear me out...

Create an abstract interface class called something like PERIPHERAL, and in this class we get the functions like read, write, whatever else is needed. We then extend this class to SPI, QUAD_SPI, UART, etc. Note about optimization if that is a concern, you might think that vtables would slow things down but if the derived classes are marked final then vtables are optimized away. Then we have a sensor interface class that has the read function, and whatever else we really want but it seems like read is the only general function we need. Then we can extend that to each sensor type like bmi088 and the other ones. The constructor for those can take a PERIPHERAL, and we use that for reading and such.

Just a note, cpp is very standard in embedded. There is really nothing to lose by using cpp, we just need to be careful about nothing going too far, but I feel like this is a happy medium use of it. If you would all like to put my head on a stick for this though that's fine, I just want to bring this idea up. Additionally, we get things like static_assert and also can be way more particular about what type of casting we are doing. Also the constexpr keyword is just excellent.

If this is something you guys would consider, I would be okay taking the lead in looking into it (unless you guys want to). I am just a fan of cpp and I think it could clean up a lot of our code. Also note that the HAL is the same for cpp and c, so that doesn't change.

To clear up what the workflow for this might be like, I am thinking that it might be like the following:
In the actual device, you would have SPI gyro_spi{&hspi1, gyro_cs_port, gyro_cs_pin) then BMI_088 bmi{gyro_spi, acc_spi}, then you would have an array with your sensors so std::array<SENSOR, NUM_SENSORS> sensors{bmi, bmp, mag}. Then in the task we can just walk through each sensor and do the reading and stuff. SENSOR will also be a very thin abstract template class (for CRTP) that would just have the read function that would just be

packet read()
{
  return static_cast<Derived*>(this)->do_read();
} 

To add on one more thing, I just want to note how this meets the goals you listed above. The different peripheral classes like SPI and QUAD_SPI can be made such that they only exist if those peripherals are enabled. If someone tries to setup a SPI instance when there is no spi enabled, they will find it difficult because SPI won't be defined. When it comes to choosing between protocols, each sensor can just take in PERIPHERAL and use its functions, so the sensor won't really care what is being passed in. To be sure there is no funny business like a UART being passed into a bmi though, we can add a peripheral type function to the PERIPHERAL class that returns the type of peripheral so we can check if it is supported. For the maintainability and readability side, writing sensors should be easier since they just take a peripheral and use the generic writes, reads, and commands in the abstract class and the correct functions will be called since we actually pass in the derived peripheral class.

So I am not opposed to C++, but that is something you have to talk to Dhruv about. Your proposed architecture is already implemented in this PR but in C (I think that is what you intended?). Our analog of your PERIPHERAL is serial_api and the constructors of each sensors do take in an instance serial_api struct which they use for reads, and writes through api->write. And the sensor reading is currently how you described via

enum sensors {
  BMP581,
  BMI088,
  NUMBER_SENSORS
};
struct sensor sensors[NUMBER_SENSORS];

for (int i = 0; i < NUMBER_SENSORS; ++i) {
        if (sensors_res[i]) {
          sensors[i].read(sensors[i].ctx, &packet);
        }
}

Of course C++ will make this more clear and readable with classes, but again, this is something you should discuss with Dhruv first. I will say that moving to C++ will give us a more standardized unit testing framework via gTest.

Yes that was more or less the idea, to replicate what you have but in cpp. I suppose this is more of a personal preference, but I do think the cpp version of doing this is more clear and readable, while maintaining speed. Lets see if dhruv would be available to talk sometime.